-
Notifications
You must be signed in to change notification settings - Fork 622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: resharding - tests for missing chunks #10052
Conversation
a4dd5fb
to
c6e8702
Compare
for chunk in chunks.iter() { | ||
assert_ne!(chunk.height_included(), last_block.header().height()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check was overly ambitious for V2. When testing the V1 resharding, it's all or nothing when it comes to chunk production. In the V2 resharding, which happens to include ChunkOnlyProducers
, chunks in a block can be produced by different chunk producers and so some can be present and some missing.
I changed it to have the same condition for both cases, when the shard layout is the same and when it is different. The condition now is that the shards with ids target_shard_ids
are missing in all blocks from block_hash
to last_block_hash_with_empty_chunk
.
if path.exists() { | ||
std::fs::remove_dir_all(&path)? | ||
} | ||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not relevant to the rest of the PR, without this check it would print some nasty errors
chain/client/src/client.rs
Outdated
debug!(target: "client", "{:?} Producing block at height {}, parent {} @ {}, {} new chunks: {:?}", validator_signer.validator_id(), | ||
next_height, prev.height(), format_hash(head.last_block_hash), new_chunks.len(), new_chunks.keys().sorted()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not relevant to the rest of the PR, but useful debug info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make it friendly to grafana tempo:
debug!(target: "client", next_height, prev_height = prev.height(), parent, ..., "Producing block");
chain/client/src/client.rs
Outdated
debug!(target: "client", "{:?} Producing block at height {}, parent {} @ {}, {} new chunks: {:?}", validator_signer.validator_id(), | ||
next_height, prev.height(), format_hash(head.last_block_hash), new_chunks.len(), new_chunks.keys().sorted()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make it friendly to grafana tempo:
debug!(target: "client", next_height, prev_height = prev.height(), parent, ..., "Producing block");
Oh great, I ran into this couple days ago and almost started thinking about a fix. |
Not sure what exactly is "this" but I'm happy to have helped :) |
I want to add a bunch of similar tests for which no resharding is happening. It is pretty convenient check of outgoing receipts handling for stateless validation. But because of broken override it didn't work for latest protocol versions before, now it should work without patches. |
get_next_block_hash_with_new_chunk
that didn't work for the V2 resharding